Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce a "valuehead" search algorithm. #2121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Introduce a "valuehead" search algorithm. #2121

wants to merge 2 commits into from

Conversation

mooskagh
Copy link
Member

@mooskagh mooskagh commented Feb 5, 2025

Instead of it being an option.

@mooskagh
Copy link
Member Author

mooskagh commented Feb 5, 2025

I'll need to fix the registration, currently it's

../src/search/instamove/instamove.cc:148:1: error: redefinition of 'reg3b50Yalgorithm'
  148 | REGISTER_SEARCH(ValueHeadFactory);
      | ^
../src/search/register.h:65:34: note: expanded from macro 'REGISTER_SEARCH'
   65 |   static SearchManager::Register reg3b50Y##algorithm(std::make_unique<alg>()); \
      |                                  ^
<scratch space>:244:1: note: expanded from here
  244 | reg3b50Yalgorithm
      | ^
../src/search/instamove/instamove.cc:147:1: note: previous definition is here
  147 | REGISTER_SEARCH(PolicyHeadFactory);
      | ^
../src/search/register.h:65:34: note: expanded from macro 'REGISTER_SEARCH'
   65 |   static SearchManager::Register reg3b50Y##algorithm(std::make_unique<alg>()); \
      |                                  ^
<scratch space>:243:1: note: expanded from here
  243 | reg3b50Yalgorithm
      | ^

@mooskagh
Copy link
Member Author

mooskagh commented Feb 6, 2025

Fixed the registration, ready for review.

borg323
borg323 reviewed Feb 17, 2025
View reviewed changes
src/search/instamove/instamove.cc
const std::vector<Move> legal_moves = board.GenerateLegalMoves();
std::vector<EvalResult> results(legal_moves.size());

for (size_t i = 0; i < legal_moves.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop should avoid exceeding the maximum_batch_size, the earlier version did.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For that we'll need to have a Backend layer that would handle this (and add SearchCapabilities to tell whether it needs that layer).
I'll send a separate PR for that (to be merged before this).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge it as is and do the other PR later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually now I have an idea how to do it without SearchCapabilities -- will update this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borg323 borg323 borg323 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mooskagh @borg323